-
Notifications
You must be signed in to change notification settings - Fork 13
Add Makefile with containerized builds #80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: empovit The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds a new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Makefile`:
- Around line 116-121: The deploy target's helm command (in the Makefile target
named "deploy") should include the --create-namespace flag so fresh clusters
succeed when the namespace (NAMESPACE) does not exist; update the helm upgrade
--install invocation that uses $(RELEASE_NAME), $(NAMESPACE),
$(REGISTRY)/$(IMAGE_NAME) and $(IMAGE_TAG) to add --create-namespace so the
namespace is created automatically on first deploy.
🧹 Nitpick comments (1)
Makefile (1)
53-83: Avoid root-owned artifacts from container runs.Running as root inside the dev image can leave root-owned files (e.g.,
dist/, cache, lockfiles) on the host, which is a common source of permission friction. Consider mapping the container user to the host UID/GID and setting a writableHOME.🔧 Suggested change (apply to build/lint/type-check/i18n)
build: ## Build plugin for production `@echo` "$(GREEN)Building plugin (containerized)...$(RESET)" @$(CONTAINER_TOOL) run --rm \ -v "$(CURDIR):/workspace:z" \ -w /workspace \ + --user $(shell id -u):$(shell id -g) \ + -e HOME=/tmp \ $(DEV_IMAGE) \ sh -c "yarn install --frozen-lockfile && yarn build" lint: ## Lint the code `@echo` "$(GREEN)Linting code (containerized)...$(RESET)" @$(CONTAINER_TOOL) run --rm \ -v "$(CURDIR):/workspace:z" \ -w /workspace \ + --user $(shell id -u):$(shell id -g) \ + -e HOME=/tmp \ $(DEV_IMAGE) \ sh -c "yarn install --frozen-lockfile && yarn lint" type-check: ## Check TypeScript types `@echo` "$(GREEN)Type checking (containerized)...$(RESET)" @$(CONTAINER_TOOL) run --rm \ -v "$(CURDIR):/workspace:z" \ -w /workspace \ + --user $(shell id -u):$(shell id -g) \ + -e HOME=/tmp \ $(DEV_IMAGE) \ sh -c "yarn install --frozen-lockfile && yarn type-check" i18n: ## Generate i18n translations `@echo` "$(GREEN)Generating i18n (containerized)...$(RESET)" @$(CONTAINER_TOOL) run --rm \ -v "$(CURDIR):/workspace:z" \ -w /workspace \ + --user $(shell id -u):$(shell id -g) \ + -e HOME=/tmp \ $(DEV_IMAGE) \ sh -c "yarn install --frozen-lockfile && yarn i18n"
Provides build, lint, type-check, i18n targets that run inside containers via Dockerfile.dev - no Node.js required on host. Includes container image and Helm deployment targets.
Provides build, lint, type-check, i18n targets that run inside containers
via Dockerfile.dev - no Node.js required on host. Includes container image
and Helm deployment targets.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.